Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate form verification on the backend #392

Merged
merged 14 commits into from
Dec 19, 2023
Merged

Validate form verification on the backend #392

merged 14 commits into from
Dec 19, 2023

Conversation

Awesome-E
Copy link
Member

@Awesome-E Awesome-E commented Dec 18, 2023 •
edited
Loading

Description (basically the commit history)

  • Instead of a simple boolean flag, use a state for the token of the captcha
  • Submit the captcha token with the POST request to /reviews
  • Validate the form body's recaptcha token the server side (REQUIRES ENV VAR TO BE UPDATED)
  • Update types on server side for ReviewData (kinda just slapped this into types.d.ts for now as we will want to refactor this anyways once we have zod and/or mongoose to validate items going into a database)

Screenshots

Mostly backend validation, but we alert the API-provided error if it exists:
image

Steps to verify/test this change:

  • Update staging and prod ENV variables with GRECAPTCHA_SECRET=<recaptcha_secret_here>
  • Verify changes work as expected on staging instance
    1. Open dev tools to the network tab
    2. Submit a legitimate review (checks that legitimate reviews still pass through)
    3. Right click the request to /reviews, copy as fetch
    4. Delete the review you just posted, so that we don't test the no duplicate reviews check
    5. Paste into the dev tools console (should be the fetch you just copied)
    6. Enter it, and watch the network tab.
    • Expected: the request gives a 400 error because the captcha token has already been used
    • It should also give a 400 error if the captchaToken is omitted form the request body
  • [] (optional) check what happens when you try setting a review to be verified: true. It should still show up in reviews to verify. This is less important because we will be validating the entire form body in the near future (Zod or mongoose).

Final Checks:

  • Verify successful deployment
  • Delete branch

(optional)

  • Write tests
  • Write documentation

Issues

Closes #381

@Awesome-E Awesome-E requested a review from js0mmer December 18, 2023 04:43
@Awesome-E
Copy link
Member Author

btw I'm not sure what other PRs open are modifying different parts of the same file, so I ignored the prettier auto-reformat file for now. I know it's not ideal, but I would rather be able to merge the other PRs (then run a larger reformat) so we can avoid merge conflicts

@js0mmer
Copy link
Member

js0mmer commented Dec 18, 2023

btw I'm not sure what other PRs open are modifying different parts of the same file, so I ignored the prettier auto-reformat file for now. I know it's not ideal, but I would rather be able to merge the other PRs (then run a larger reformat) so we can avoid merge conflicts

That's fine.

You'll need to update stacks/backend.ts to include the GRECAPTCHA_SECRET env variable.

@Awesome-E
Copy link
Member Author

@js0mmer added!

@Awesome-E Awesome-E requested review from js0mmer and removed request for js0mmer December 18, 2023 18:14
js0mmer
js0mmer reviewed Dec 18, 2023
View reviewed changes
Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't seem to get it to work on staging despite setting up the env variable. Says ReCAPTCHA token is invalid when just checking it and submitting normally.

@Awesome-E
Copy link
Member Author

That's strange – are you able to get it to work locally and/or did you redeploy after changing the env vars?

@Awesome-E
Copy link
Member Author

or maybe from Google Recaptcha manage page you need to allow it to be on any *.peterportal.org subdomain?

@js0mmer
Copy link
Member

js0mmer commented Dec 18, 2023

Subdomains are already added by default.
image
I have redeployed already and initially set up the env variable last night. Haven't tested locally yet, will look into this later tonight.

@js0mmer
Copy link
Member

js0mmer commented Dec 18, 2023

Actually I see, there's one more spot you need to add the env variable, in the build-and-deploy workflow under the .github folder.

@Awesome-E
Copy link
Member Author

Alright cool found that and one more spot where there were env vars. Hopefully it is correct now

@Awesome-E
Copy link
Member Author

For some reason it still doesn't seem to work – is it in the GitHub repo secrets?

@js0mmer
Copy link
Member

js0mmer commented Dec 18, 2023

Yes should be. Entered it here and verified it was correct this morning.

image

Would you be able to change the server to respond with the error message from reCAPTCHA to help debug the problem. Despite the console.err you put, it does not appear in the AWS cloudwatch logs.

@Awesome-E
Copy link
Member Author

Awesome-E commented Dec 18, 2023 •
edited
Loading

The catch statement doesn't include instances where tokens are invalid, so that's why it didn't log. (the request itself didn't fail, it's just that the success property was false, so there's no log)
I returned the Google recaptcha response in the /review response for now so you can see it through dev tools, but we might also consider console logging the response in the server whenever a validation fails as well.

@Awesome-E Awesome-E requested a review from js0mmer December 18, 2023 22:47
js0mmer
js0mmer requested changes Dec 18, 2023
View reviewed changes
Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on staging and locally! See additional comments.

api/src/helpers/recaptcha.ts Outdated Show resolved Hide resolved
api/src/helpers/recaptcha.ts Outdated Show resolved Hide resolved
api/src/controllers/reviews.ts Outdated Show resolved Hide resolved
@Awesome-E @js0mmer
Simplify query string
ef0c45a 
Co-authored-by: Jacob Sommer <[email protected]>
@Awesome-E Awesome-E requested a review from js0mmer December 18, 2023 23:10
js0mmer
js0mmer approved these changes Dec 18, 2023
View reviewed changes
Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more areas with ||. Other than that, looks good.

site/src/component/ReviewForm/ReviewForm.tsx Outdated Show resolved Hide resolved
site/src/component/ReviewForm/ReviewForm.tsx Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Deployed staging instance to https://staging-392.peterportal.org

@Awesome-E Awesome-E merged commit 6afad12 into master Dec 19, 2023
2 checks passed
@Awesome-E Awesome-E deleted the form-verification branch December 19, 2023 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@js0mmer js0mmer js0mmer approved these changes

Assignees

@Awesome-E Awesome-E

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verify review form reCAPTCHA on backend
2 participants
@Awesome-E @js0mmer